Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: improve StreamManager #1994

Merged
merged 11 commits into from
May 14, 2024
Merged

chore: improve StreamManager #1994

merged 11 commits into from
May 14, 2024

Conversation

danisharora099
Copy link
Collaborator

@danisharora099 danisharora099 commented Apr 30, 2024

Problem

#1965

Solution

This PR:

  • adds connection timeout, retry backoff base and maximum retries for stream creation
  • sets up a Promise.race() between successful stream creation and timeout
  • adds error handling to getStream to log errors when stream creation fails
  • handles stream creation based on connection status

Notes

Contribution checklist:

  • covered by unit tests;
  • covered by e2e test;
  • add ! in title if breaks public API;

Copy link

github-actions bot commented Apr 30, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 181.37 KB (+0.08% 🔺) 3.7 s (+0.08% 🔺) 3.1 s (-39.28% 🔽) 6.7 s
Waku Simple Light Node 181.54 KB (+0.13% 🔺) 3.7 s (+0.13% 🔺) 3.8 s (+64.87% 🔺) 7.4 s
ECIES encryption 23.12 KB (+0.01% 🔺) 463 ms (+0.01% 🔺) 942 ms (+11.99% 🔺) 1.5 s
Symmetric encryption 22.58 KB (+0.23% 🔺) 452 ms (+0.23% 🔺) 1.1 s (+3.27% 🔺) 1.6 s
DNS discovery 72.49 KB (-0.02% 🔽) 1.5 s (-0.02% 🔽) 2.5 s (-3.03% 🔽) 3.9 s
Peer Exchange discovery 74.15 KB (+0.1% 🔺) 1.5 s (+0.1% 🔺) 2.6 s (+25.74% 🔺) 4.1 s
Local Peer Cache Discovery 67.68 KB (+0.08% 🔺) 1.4 s (+0.08% 🔺) 2.2 s (-5.33% 🔽) 3.6 s
Privacy preserving protocols 38.87 KB (-0.04% 🔽) 778 ms (-0.04% 🔽) 1.9 s (-18.72% 🔽) 2.6 s
Waku Filter 112.02 KB (+0.28% 🔺) 2.3 s (+0.28% 🔺) 3.1 s (+12.8% 🔺) 5.3 s
Waku LightPush 110.39 KB (+0.13% 🔺) 2.3 s (+0.13% 🔺) 3.4 s (+0.91% 🔺) 5.6 s
History retrieval protocols 110.91 KB (+0.14% 🔺) 2.3 s (+0.14% 🔺) 3.2 s (+24.81% 🔺) 5.5 s
Deterministic Message Hashing 7.29 KB (+0.71% 🔺) 146 ms (+0.71% 🔺) 358 ms (-26.31% 🔽) 504 ms

@danisharora099 danisharora099 marked this pull request as ready for review April 30, 2024 12:31
@danisharora099 danisharora099 requested a review from a team as a code owner April 30, 2024 12:31
}

private prepareNewStream(peer: Peer): void {
const streamPromise = this.newStream(peer).catch(() => {
// No error thrown as this call is not triggered by the user
const timeoutPromise = new Promise<void>((resolve) =>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make it a util or use some small npm module for it (I prefer to have some convenient module) - we can do a follow up

return connection.newStream(this.multicodec);

try {
return await connection.newStream(this.multicodec);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's rewrite to

let result;
try {
  result = await ...;
}
return result;

the reason is due to huge difference in code execution because of return or return await

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for other places

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you elaborate on the difference in code execution? imo both of them are the same?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if try { return promise } catch ... - it won't catch as promise is not awaited
if try { return await promise } catch ... - will catch and the difference is only in one word

I think it is better to be on a clear side of things and write:

let result;
try {
  result = await ...;
}
return result;

as the absence of await changes so much (and I faced some nasty bugs in the past because of that)

@weboko
Copy link
Collaborator

weboko commented Apr 30, 2024

How does this PR address the main issue? Could you share knowledge about it, please.

@danisharora099 danisharora099 self-assigned this May 6, 2024
@danisharora099 danisharora099 requested a review from weboko May 7, 2024 06:46
@danisharora099
Copy link
Collaborator Author

How does this PR address the main issue? Could you share knowledge about it, please.

My suspicion is that stream creation process and peer disconnection process are getting into a race condition where:

  • a peer connects
  • stream creation is initiated
  • peer disconnects (for whatever reason: eg: metadata mismatch)
  • stream creation fails

This PR:

  • races between stream creation and error (connection timeout) that would help us understand what the reason for stream creation failure is
  • limits number of retries for stream creation by a maximum retry, and backoff base
  • adds more logs

@@ -91,7 +91,12 @@ export class FilterCore extends BaseProtocol implements IBaseProtocolCore {
peer: Peer,
contentTopics: ContentTopic[]
): Promise<void> {
const stream = await this.getStream(peer);
let stream: Stream;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: as this pattern repeats 3 times already in the file - let's split it into private method

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or because it is repeated in many other places - we can include it into .getStream implementation

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include which part?

we will have to handle errors on Filter side as well

Copy link
Collaborator

@weboko weboko May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant this code

try {
      stream = await this.getStream(peer);
    } catch (error) {
      throw new Error(`Failed to get stream for peer ${peer.id.toString()}`);
    }
    ```
it is ok to let `getStream` fail with message and pop up till we want to handle it, we don't need to handle it each on each level, because we are basically doing re-throw 

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha. this is now outdated code -- we now return error code instead of throwing

@danisharora099 danisharora099 force-pushed the chore/improve-stream-man branch 2 times, most recently from 62ac073 to 3ec2344 Compare May 7, 2024 13:48
@danisharora099 danisharora099 requested a review from weboko May 9, 2024 10:42
Copy link
Collaborator

@weboko weboko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some open comments + validation with light-js but overall looks good!

@danisharora099 danisharora099 merged commit e49e728 into master May 14, 2024
11 checks passed
@danisharora099 danisharora099 deleted the chore/improve-stream-man branch May 14, 2024 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants